Add API to link/unlink provider info to/from user account. #383
Conversation
| @@ -532,7 +532,8 @@ def create_user(self, uid=None, display_name=None, email=None, phone_number=None | |||
|
|
|||
| def update_user(self, uid, display_name=None, email=None, phone_number=None, | |||
| photo_url=None, password=None, disabled=None, email_verified=None, | |||
| valid_since=None, custom_claims=None): | |||
| valid_since=None, custom_claims=None, link_provider=None, | |||
There was a problem hiding this comment.
The names have changed since starting this (and may again; feel free to defer fixing this until the name's are finalized). But as of now:
- s/link_provider/provider_to_link/
- s/delete_provider_ids/providers_to_delete/
| @@ -541,6 +542,8 @@ def update_user(self, uid, display_name=None, email=None, phone_number=None, | |||
| 'validSince': _auth_utils.validate_timestamp(valid_since, 'valid_since'), | |||
| 'emailVerified': bool(email_verified) if email_verified is not None else None, | |||
| 'disableUser': bool(disabled) if disabled is not None else None, | |||
| 'linkProviderUserInfo': link_provider.to_dict() if link_provider is not None else None, | |||
There was a problem hiding this comment.
May be worth validating this parameter? _auth_utils.validate_user_provider() or similar?
| @@ -388,6 +388,8 @@ def update_user(uid, **kwargs): | |||
| user account (optional). To remove all custom claims, pass ``auth.DELETE_ATTRIBUTE``. | |||
| valid_since: An integer signifying the seconds since the epoch. This field is set by | |||
| ``revoke_refresh_tokens`` and it is discouraged to set this field directly. | |||
| link_provider: User's provider info to be linked to the user account. | |||
| @@ -388,6 +388,8 @@ def update_user(uid, **kwargs): | |||
| user account (optional). To remove all custom claims, pass ``auth.DELETE_ATTRIBUTE``. | |||
| valid_since: An integer signifying the seconds since the epoch. This field is set by | |||
| ``revoke_refresh_tokens`` and it is discouraged to set this field directly. | |||
| link_provider: User's provider info to be linked to the user account. | |||
There was a problem hiding this comment.
We should indicate the type here. It's especially important in python, since (absent type annotations) it can be non-trivial to figure that out for the user. In this case, maybe "A UserProvider instance that contains the user's provider info to be linked to the user account." ?
| user = auth.update_user( | ||
| new_user.uid, | ||
| link_provider=auth.UserProvider( | ||
| uid='test', provider_id='google.com', email='[email protected]', |
There was a problem hiding this comment.
Caution: I think that a single provider_uid can only be linked to a single account. So by linking provideruid='test' to this account, unless the account is deleted, the second (and all subsequent) time you run this test, it'll fail. That's mostly ok here though, since the way the new user is being setup, it'll automatically delete itself, even if the test fails. But you could imagine the CI system running kill -9 (or hard power-off, or ...) right in the middle of this test. Highly unlikely... but possible.
A few options:
a) Don't worry about it; it's pretty improbable, and we can fix by hand if we run into it.
b) Instead of using a hardcoded uid ("test") create a random one. (Which makes it another order of magnitude less likely to bite us, even if it doesn't quite eliminate the possibility altogether.)
c) At the start of the test, find the user with provider_id="google.com" and provider_uid="test". Either delete or unlink that user if it exists. (But still delete the user at the end of the test too.)
I'd prefer either b or c.
| uid='test', provider_id='google.com', email='[email protected]', | ||
| display_name='Test Name', photo_url='https://test.com/user.png')) | ||
| assert user.uid == new_user.uid | ||
| assert len(user.provider_data) == 3 |
There was a problem hiding this comment.
This is no worse than what was there before. But consider validating the contents of this list instead. Validating the entire instance might be overkill, but how about:
assert sorted(map(lambda user_info: user_info.provider_id, user.provider_data)) == sorted(["email", "phone", "google.com"])
(untested, and probably too terse. A helper would likely make it more readable.)
| delete_provider_ids=['google.com']) | ||
| assert user.uid == new_user.uid | ||
| assert user.phone_number is None | ||
| assert len(user.provider_data) == 1 |
There was a problem hiding this comment.
If you make a change above, do so here (and below) as well.
| assert user.uid == new_user.uid | ||
| assert user.phone_number is None | ||
| assert len(user.provider_data) == 1 | ||
|
|
There was a problem hiding this comment.
A new requirement is that this API should also work with non-federated providers. (It's not something we're going to actually recommend, since there's more direct ways to do that.) Unfortunately, the backend for this API doesn't support non-federated providers, so we're going to have to fake that a bit.
Concretely, you've used update_user() to link to the 'google.com' provider. Could you also add calls for phone, email, oauth and saml? (Not necessarily in this exact test.) It'll require you to also change the implementation.
If this looks like it'll cause this PR to balloon in size, then let's talk first and see if we can come up with other options.
| 'photoUrl': 'https://test.com/user.png' | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Consider adding "failure" tests.
a) empty delete_providers_ids list
b) invalid provider_id. (I'm not sure we can tell if a provider_id is valid or not... other than checking for None or empty string.)
c) invalid provider_uid. (Same; just None/empty checks.)
These should all cause failures before hitting the network, so are easy to check from a unit test.
No description provided.